Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support for multiple types of workloads #15529

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Support for multiple types of workloads #15529

wants to merge 2 commits into from

Conversation

chenzhiguo
Copy link

Fixes #15528

Release Note

Enhance the scalability to adapt to different workloads.

@knative-prow Knative Prow
Copy link

knative-prow bot commented Sep 25, 2024

Welcome @chenzhiguo! It looks like this is your first PR to knative/serving 🎉

@knative-prow Knative Prow
Copy link

knative-prow bot commented Sep 25, 2024

Hi @chenzhiguo. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@knative-prow knative-prow bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 25, 2024
@knative-prow Knative Prow
Copy link

knative-prow bot commented Sep 25, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chenzhiguo
Once this PR has been reviewed and has the lgtm label, please assign psschwei for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@chenzhiguo chenzhiguo changed the title Support for multiple types of workloads for #15528 Support for multiple types of workloads Sep 25, 2024
skonto
skonto reviewed Sep 25, 2024
View reviewed changes
pkg/reconciler/autoscaling/kpa/scaler.go
@@ -372,5 +373,30 @@ func (ks *scaler) scale(ctx context.Context, pa *autoscalingv1alpha1.PodAutoscal
}

logger.Infof("Scaling from %d to %d", currentScale, desiredScale)
return desiredScale, ks.applyScale(ctx, pa, desiredScale, ps)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems you still have func (ks *scaler) applyScale(ctx context.Context... left unused.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it can be delete.

@skonto
Copy link
Contributor

skonto commented Sep 25, 2024 •
edited
Loading

Hi @chenzhiguo, I think there should be a design doc for this type of change (as this PR seems part of a bigger set of changes). Knative does not pick up automatically a new kind for autoscaling. For example when a revision is reconciled we create a deployment and that is the only option right now. We have status reporting based on the deployment resource etc. cc @dprotaso

skonto
skonto reviewed Sep 25, 2024
View reviewed changes
pkg/reconciler/autoscaling/interface.go
import (
"context"
"fmt"
v1 "k8s.io/api/core/v1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a new line in between.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skonto skonto skonto left review comments

@izabelacg izabelacg Awaiting requested review from izabelacg

Assignees
No one assigned
Labels
needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enhance the scalability of multi-type workloads
2 participants
@chenzhiguo @skonto